Skip to content

stringsconcatloop: detect x = x + y accumulator pattern alongside += - #49066

Merged
pelikhan merged 6 commits into
mainfrom
copilot/fix-stringsconcatloop-linter-bypass
Jul 30, 2026
Merged

stringsconcatloop: detect x = x + y accumulator pattern alongside +=#49066
pelikhan merged 6 commits into
mainfrom
copilot/fix-stringsconcatloop-linter-bypass

Conversation

Copilot AI commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

The linter only matched token.ADD_ASSIGN (x += y), leaving the semantically identical x = x + y form undetected — a gap that was being intentionally exploited as a documented workaround.

Changes

  • New pattern match: run() now also handles token.ASSIGN where Rhs[0] is a *ast.BinaryExpr{Op: token.ADD} whose left operand is the same identifier as Lhs[0] (direct self-referential form only — x = x + a + b is out of scope).

  • Loop-scope guard: For the ASSIGN form, variables declared by the enclosing loop itself are excluded — range key/value vars and for-init short-decl vars are per-iteration rebinds, not cross-iteration accumulators. This avoids false positives like:

    for _, line := range lines {
        line = line + "  # comment: " + extra  // not flagged — line is reset each iteration
    }
  • enclosingLoopPositionenclosingLoop: refactored to also return the ast.Node so isLoopScopedIdent can inspect the loop's key/value/init declarations.

  • isLoopScopedIdent: new helper that checks whether a name is declared by a RangeStmt (Key/Value) or ForStmt (:= Init).

  • Testdata: added true-positive cases for both ASSIGN forms (range loop and classic for), plus false-positive guard cases (range value var, range key var over map[string]int, func-literal boundary, non-self-referential accum = p + accum, out-of-loop form).

  • doc.go / Analyzer.Doc: updated descriptions to reflect both += and x = x + y.


Run: https://github.com/github/gh-aw/actions/runs/30523835792

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 4.89 AIC · ⌖ 7.33 AIC · ⊞ 8.7K ·
Comment /souschef to run again

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix stringsconcatloop linter to match assignment expression stringsconcatloop: detect x = x + y accumulator pattern alongside += Jul 30, 2026
Copilot AI requested a review from pelikhan July 30, 2026 06:34
@pelikhan
pelikhan marked this pull request as ready for review July 30, 2026 06:45
Copilot AI review requested due to automatic review settings July 30, 2026 06:45
@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

PR Code Quality Reviewer completed the code quality review.

@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Test Quality Sentinel completed test quality analysis.

Warning

threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.

Details

The threat detection engine failed to produce results.

Review the workflow run logs for details.

No test files were added or modified in this PR. PR changes only the linter implementation (stringsconcatloop.go), documentation, and test fixture data (testdata/src/.../stringsconcatloop.go). The existing stringsconcatloop_test.go file remains unchanged. Test Quality Sentinel skipped.

@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Design Decision Gate 🏗️ failed during design decision gate check.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Not ready to approve

The scope guard misses valid classic-for accumulators, and the new diagnostic inaccurately reports += syntax.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

Extends stringsconcatloop to detect direct x = x + y string accumulators.

Changes:

  • Adds assignment-form detection and loop-scope filtering.
  • Adds positive and negative analyzer test cases.
  • Updates linter documentation.
File summaries
File Description
pkg/linters/stringsconcatloop/stringsconcatloop.go Implements detection and scope handling.
pkg/linters/stringsconcatloop/testdata/src/stringsconcatloop/stringsconcatloop.go Adds analyzer test cases.
pkg/linters/doc.go Updates the linter summary.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 2
  • Review effort level: Medium

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

// x = x + y form in a range loop – should be flagged.
accum := ""
for _, p := range parts {
accum = accum + p // want `string concatenation with \+= inside a loop`

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in commit 4c74e3e. The diagnostic message is now syntax-neutral: "string concatenation inside a loop allocates O(n) strings and O(n²) total bytes; use strings.Builder instead". All // want patterns in testdata updated accordingly.

Comment on lines +135 to 158
// isLoopScopedIdent reports whether name is declared by loopNode as a loop
// variable: the Key or Value identifier of a RangeStmt, or a variable in the
// short-declaration Init of a ForStmt. Such variables are per-iteration
// rebinds, not cross-iteration accumulators.
func isLoopScopedIdent(loopNode ast.Node, name string) bool {
switch n := loopNode.(type) {
case *ast.RangeStmt:
if id, ok := n.Key.(*ast.Ident); ok && id.Name == name {
return true
}
if id, ok := n.Value.(*ast.Ident); ok && id.Name == name {
return true
}
case *ast.ForStmt:
init, ok := n.Init.(*ast.AssignStmt)
if !ok || init.Tok != token.DEFINE {
return false
}
for _, lhs := range init.Lhs {
if id, ok := lhs.(*ast.Ident); ok && id.Name == name {
return true
}
}
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in commit 4c74e3e. Removed the ForStmt branch from isLoopScopedIdent — for-init variables run once before the loop and carry state across all iterations, so they are genuine accumulators and must be flagged. Only RangeStmt Key/Value remain exempted. Added a regression test case for for s3 := ""; ... that verifies it is now flagged.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation is clean and correct. The x = x + y accumulator detection is well-guarded:

  • Requires exact self-referential form (x = x + rhs), not x = rhs + x — intentionally conservative.
  • isLoopScopedIdent correctly handles nil Key/Value/Init via type assertions.
  • Test cases cover all relevant branches (range, for, func-literal boundary, loop-scoped vars, outside-loop, reversed operand).

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 15.6 AIC · ⌖ 4.75 AIC · ⊞ 5.3K

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skills-Based Review 🧠

Applied /diagnosing-bugs and /tdd — requesting changes on a correctness gap and two missing test boundaries.

📋 Key Themes & Highlights

Key Issues

  • Correctness gap (line 105): The isLoopScopedIdent guard is only applied to the ASSIGN form. A range-value or for-init variable using += (e.g. line += suffix) would be falsely flagged. The two forms need consistent treatment.
  • Missing test (line 75): The out-of-scope chained form x = x + a + b is called out in a comment but not covered by a testdata case. The boundary is implicitly correct today but unverified.
  • Asymmetric defensive guard (line 56): The len(assign.Lhs) != 1 check exists only for ASSIGN, not ADD_ASSIGN. Keeping them symmetric makes future audits easier.

Positive Highlights

  • ✅ Clean refactor of enclosingLoopPositionenclosingLoop — returning the AST node is the right design for the new guard.
  • isLoopScopedIdent is a well-scoped, readable helper with a clear contract.
  • ✅ Comprehensive false-positive cases in testdata: reversed operands, func-literal boundary, out-of-loop form, range key/value vars.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 30.3 AIC · ⌖ 5.18 AIC · ⊞ 7K
Comment /matt to run again

// For the x = x + y form, skip variables declared by the enclosing
// loop itself (range key/value or for-init short-decl): those are
// per-iteration rebinds, not cross-iteration accumulators.
if assignLhsName != "" && isLoopScopedIdent(loopNode, assignLhsName) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The loop-scope guard is only applied to the ASSIGN form (x = x + y), not to ADD_ASSIGN (x += y). A range-value or for-init variable using += would be falsely flagged, creating a behavioral asymmetry between the two forms.

💡 Example false positive (currently unflagged by tests)
for _, line := range parts {
    line += " suffix" // would be flagged — but line is a per-iteration rebind
}

The fix: capture the LHS name for ADD_ASSIGN too and apply isLoopScopedIdent uniformly, or extract a shared helper. A test case should be added to testdata to prevent regression.

@copilot please address this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in commit 89abd77. The scope guard (isLoopScopedIdent + isLoopBodyLocal) is now applied uniformly whenever the LHS is a plain *ast.Ident, regardless of whether the token is ADD_ASSIGN or ASSIGN. A testdata case for line += " suffix" on a range-value variable confirms it is no longer flagged.

var assignLhsName string // non-empty only for the token.ASSIGN form

switch assign.Tok {
case token.ADD_ASSIGN:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnosing-bugs] For ADD_ASSIGN, assign.Lhs[0] is used directly without checking that len(assign.Lhs) == 1. A multi-LHS assignment like a, b += ... is syntactically invalid in Go, but the guard on line 61–63 (applied only to ASSIGN) makes the intent asymmetric and harder to reason about.

💡 Suggestion

Add a len(assign.Lhs) == 1 guard for the ADD_ASSIGN case as well, matching the defensive style used for ASSIGN. This is belt-and-suspenders given Go syntax, but keeps both branches symmetric and easy to audit.

case token.ADD_ASSIGN:
    if len(assign.Lhs) != 1 {
        continue
    }
    lhsExpr = assign.Lhs[0]

@copilot please address this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in commit 89abd77. Added len(assign.Lhs) != 1 guard to the ADD_ASSIGN case, making it symmetric with the ASSIGN branch.

rhsLeft, ok := binExpr.X.(*ast.Ident)
if !ok || rhsLeft.Name != lhsIdent.Name {
continue
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The comment says "direct self-referential form only — x = x + a + b is out of scope", but this is not tested. The x = x + a + b case parses as BinaryExpr{X: BinaryExpr{X: x, Y: a}, Y: b}, so binExpr.X would be a *ast.BinaryExpr, not *ast.Ident — meaning it would silently fall through. A testdata case would document and lock this boundary.

💡 Suggested test case
// x = x + a + b (chained) – out of scope, not flagged.
for _, p := range parts {
    accum = accum + p + " extra" // should NOT be flagged
}

This prevents a future contributor from unexpectedly expanding the scope without realising it.

@copilot please address this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a testdata case in commit 89abd77: accum4 = accum4 + p + " extra" (chained form) is present in good() with no // want annotation, which the test framework enforces must produce no diagnostic. The comment explains why: binExpr.X is itself a BinaryExpr, not an *ast.Ident, so the self-referential check fails silently.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: Request Changes — a real false-positive gap and an asymmetric detection bypass need addressing before merge.

Themes
  • isLoopScopedIdent doesn't account for accumulator-shaped variables declared with var inside the loop body itself, so per-iteration locals can be misflagged as cross-iteration accumulators (medium, no test coverage for this case despite the PR otherwise being thorough about false-positive guards).
  • The new x = x + y matcher only supports simple identifier LHS, while the existing += matcher supports any lvalue (struct fields, map/slice indices). This reintroduces exactly the kind of detection bypass this PR set out to close, just for a narrower set of forms.
  • Core refactor (enclosingLoopPositionenclosingLoop, new isLoopScopedIdent helper) is otherwise clean and well-tested for the range/for key/value/init cases it does handle.

🔎 Code quality review by PR Code Quality Reviewer · auto · 42.4 AIC · ⌖ 4.75 AIC · ⊞ 7.8K
Comment /review to run again

// For the x = x + y form, skip variables declared by the enclosing
// loop itself (range key/value or for-init short-decl): those are
// per-iteration rebinds, not cross-iteration accumulators.
if assignLhsName != "" && isLoopScopedIdent(loopNode, assignLhsName) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

False positive: a fresh var accum string declared inside the loop body each iteration is misclassified as a cross-iteration accumulator and gets flagged.

💡 Loop-body-local var decls are not recognized as per-iteration rebinds

isLoopScopedIdent only checks RangeStmt.Key/Value and the ForStmt short-decl Init. It does not look at var declarations inside the loop body itself. Example that would be (incorrectly) flagged:

for _, p := range parts {
    var accum string
    accum = accum + p
    _ = accum
}

Here accum is declared fresh every iteration — it is not a cross-iteration accumulator, yet the current logic reports it as one because the declaration site is inside the loop body, not the RangeStmt/ForStmt node itself.

Fix: walk the loop body's top-level statement list (or use pass.TypesInfo.Defs/scope lookup) to check whether the identifier's declaring *ast.GenDecl/ValueSpec or := statement is inside the loop body, and treat that as loop-scoped too.

No test case in the diff covers this scenario, despite the PR explicitly adding several other false-positive guard tests.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in commit 89abd77. Added isLoopBodyLocal which uses pass.TypesInfo.ObjectOf(ident).Pos() to check whether the variable's declaration falls inside the loop body (body.Lbrace .. body.Rbrace). A testdata case var local string; local = local + p inside a range loop confirms it is not flagged.

switch assign.Tok {
case token.ADD_ASSIGN:
lhsExpr = assign.Lhs[0]
case token.ASSIGN:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new x = x + y detection only supports a simple identifier LHS, unlike += which accepts any lvalue — an inconsistency that silently drops coverage for common accumulator patterns.

💡 Asymmetric LHS handling between ADD_ASSIGN and ASSIGN paths

For token.ADD_ASSIGN, lhsExpr = assign.Lhs[0] accepts any expression — s.Field += y, m[k] += y, arr[i] += y are all flagged. For the new token.ASSIGN branch, assign.Lhs[0].(*ast.Ident) requires a plain identifier, so the equivalent s.Field = s.Field + y or m[k] = m[k] + y forms are silently skipped.

This means the PR's stated goal (detecting the semantically identical x = x + y form) is only partially achieved — struct-field and map/slice-index accumulators, just as susceptible to the O(n2) allocation problem, remain an undetected bypass, exactly the kind of gap this PR is meant to close.

Suggested fix: generalize the self-referential check to compare binExpr.X against assign.Lhs[0] structurally rather than restricting to *ast.Ident.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ASSIGN path intentionally restricts to plain identifiers. Generalising to struct-field and map/slice-index lvalues (s.Field = s.Field + y, m[k] = m[k] + y) requires structural AST comparison and is explicitly out of scope for this PR (noted in the PR description as "direct self-referential form only"). The ADD_ASSIGN path already catches field/index accumulators via the existing += form; this gap is narrow in practice. A follow-up issue can track the generalisation if desired.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

Triage Summary

  • Category: feature
  • Risk: low
  • Priority Score: 38/100 (impact 20 + urgency 10 + quality 8)
  • Recommended Action: defer

Linter enhancement (stringsconcatloop accumulator pattern). PR is in blocked (merge-conflict) state — needs rebase.

Generated by 🔧 PR Triage Agent · auto · 54.9 AIC · ⌖ 5.64 AIC · ⊞ 7.9K ·

@pelikhan

Copy link
Copy Markdown
Collaborator

@copilot run pr-finisher skill

@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot please run the pr-finisher skill, address unresolved review feedback if any, and rerun checks once the branch is up to date.

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 4.89 AIC · ⌖ 7.33 AIC · ⊞ 8.7K ·
Comment /souschef to run again

Copilot AI and others added 2 commits July 30, 2026 07:53
…emption

- Make diagnostic message syntax-neutral: remove '+=', now reads
  'string concatenation inside a loop ...'
- Remove ForStmt init-var exemption from isLoopScopedIdent: for-init
  vars run once and carry state across iterations (genuine accumulators),
  so they must be flagged. Only RangeStmt Key/Value remain exempted.
- Update all testdata // want patterns to match new message
- Add regression test: for-init string accumulator is flagged

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Apply isLoopScopedIdent + isLoopBodyLocal to ADD_ASSIGN form too,
  not just ASSIGN; range-value vars using += are now correctly exempted
- Add len(assign.Lhs) != 1 guard to ADD_ASSIGN case (symmetric with ASSIGN)
- Add isLoopBodyLocal helper: uses TypesInfo.ObjectOf to detect vars
  declared inside the loop body (per-iteration locals, not accumulators)
- Testdata: add chained x = x + a + b case (not flagged), loop-body-local
  var case (not flagged), and ADD_ASSIGN range-value guard case

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot AI commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

@copilot run pr-finisher skill

All review threads addressed:

  • Diagnostic message is now syntax-neutral (no += reference)
  • for-init accumulators are now correctly flagged (ForStmt branch removed from isLoopScopedIdent)
  • Scope guard applied uniformly to both += and x = x + y forms
  • len(assign.Lhs) != 1 guard added to ADD_ASSIGN case
  • isLoopBodyLocal helper added: loop-body-declared vars are not flagged
  • Testdata updated: new cases for for-init accumulator, chained form, loop-body-local var, and += range-value guard

Local make fmt and go test ./pkg/linters/stringsconcatloop/... pass.

@pelikhan
pelikhan enabled auto-merge (squash) July 30, 2026 08:02
@pelikhan
pelikhan merged commit 15376c5 into main Jul 30, 2026
1 check failed
@pelikhan
pelikhan deleted the copilot/fix-stringsconcatloop-linter-bypass branch July 30, 2026 08:03
Copilot stopped work on behalf of gh-aw-bot due to an error July 30, 2026 08:03
Copilot AI requested a review from gh-aw-bot July 30, 2026 08:03
@github-actions

Copy link
Copy Markdown
Contributor

🎉 This pull request is included in a new release.

Release: v0.84.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

4 participants